Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] enable lora for sdxl adapters too and add slow tests. #5555

Merged
merged 6 commits into from
Nov 1, 2023

Conversation

ilisparrow
Copy link
Contributor

Fixes #5516
Additionally, adds SLOW tests for SDXL T2i adapters.

Hello,
as requested here is a PR to fix the #5516, WIth it's corresponding tests.

I have a few questions :

  • The test seem to fail when this is removed from the pipeline initialization : torch_dtype=torch.float16, But your PR that inspired mine didn't have it(https://github.com/huggingface/diffusers/pull/4666/files).
    This is the error that I get with it :
    RuntimeError: Input type (float) and bias type (c10::Half) should be the same
  • I get this warning when I add .to("CPU") to the pipeline and the adapter :
    Pipelines loaded with 'torch_dtype=torch.float16' cannot run with 'cpu' device. It is not recommended to move them to 'cpu' as running them will fail. Please make sure to use an accelerator to run the pipeline in inference, due to the lack of support for'float16' operations on this device in PyTorch. Please, remove the 'torch_dtype=torch.float16' argument, or use another device for inference.
  • Last, In the tests, we load this image : https://huggingface.co/datasets/hf-internal-testing/diffusers-images/resolve/main/t2i_adapter/toy_canny.png, but if we are testing for canny of functions that process images with canny, shouldn't we input a real image and expect this as an output ? Because otherwise we are applying a canny to an already canny image.

I hope this helps.

@sayakpaul

@sayakpaul
Copy link
Member

LoRA related additions look good to me! Are there any cool results that you would like so share with us here?

For the tests, I will defer to @DN6.

Copy link
Collaborator

@DN6 DN6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏽

ilisparrow and others added 2 commits October 27, 2023 15:46
Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>
Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>
@ilisparrow
Copy link
Contributor Author

Thanks, great. Do you know when it will be merged?
I have some feedback or comments in or comment, that you might find interesting.

Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 30, 2023

The documentation is not available anymore as the PR was closed or merged.

@DN6
Copy link
Collaborator

DN6 commented Oct 31, 2023

@ilisparrow I think you would need to run make style && make quality as well as make fix-copies on the PR again.

@patrickvonplaten patrickvonplaten merged commit 5712c3d into huggingface:main Nov 1, 2023
9 of 11 checks passed
@patrickvonplaten
Copy link
Contributor

Great job @ilisparrow !

@patrickvonplaten
Copy link
Contributor

Actually this PR was very much incorrect, SDXL Adapter already had the LoRA load mixin and the tests had errors. Reverting this PR here: #5555

@ilisparrow
Copy link
Contributor Author

Hello,
Thank you for taking the time to have a look.
This PR solves this problem : #5516
That sayakpaul was able to reproduce. Where I tried to add alora to an adapter constrained SDXL model.
It was inspired by this one : #4664
Which was solving the same for controlnet and SDXL .

I might be missing something, but it fixed a real problem, and was reproduced by Sayakpaul.

@sayakpaul
Copy link
Member

That might have been the case because I reproduced it with the stable release of diffusers. Installing from main might have actually solved it:

DiffusionPipeline, FromSingleFileMixin, StableDiffusionXLLoraLoaderMixin, TextualInversionLoaderMixin

I apologize for the oversight caused on my part.

kashif pushed a commit to kashif/diffusers that referenced this pull request Nov 11, 2023
…gface#5555)

* Enable lora for sdxl adapters too.

Issue huggingface#5516

* fix: assertion values.

* Use numpy_cosine_similarity_distance on the arrays

Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>

* Use numpy_cosine_similarity_distance on the arrays

Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>

* Changed imports orders to pass tests

Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>

---------

Co-authored-by: Ilias A <iliasamri00@gmail.com>
Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>
Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
…gface#5555)

* Enable lora for sdxl adapters too.

Issue huggingface#5516

* fix: assertion values.

* Use numpy_cosine_similarity_distance on the arrays

Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>

* Use numpy_cosine_similarity_distance on the arrays

Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>

* Changed imports orders to pass tests

Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>

---------

Co-authored-by: Ilias A <iliasamri00@gmail.com>
Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>
Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
…gface#5555)

* Enable lora for sdxl adapters too.

Issue huggingface#5516

* fix: assertion values.

* Use numpy_cosine_similarity_distance on the arrays

Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>

* Use numpy_cosine_similarity_distance on the arrays

Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>

* Changed imports orders to pass tests

Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>

---------

Co-authored-by: Ilias A <iliasamri00@gmail.com>
Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>
Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lora not functioning when used with t2i adapters Pipeline
6 participants